Repository navigation
Refactor Runtime, Session, and output types - #3
Merged
Merged
Conversation
…d flexibility, enhance documentation around ownership and lifecycle policies, and add comprehensive tests for content handling, error forwarding, and JSON serialization.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The lifecycle contract cannot reliably combine terminal and close errors without either duplication or loss.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Refactors execution output into one-shot handles with detached content, updating debugger events, serialization, documentation, and tests.
Changes:
- Adds
Outputconsumption and collection contracts. - Introduces detached
Content, metadata, and lifecycle errors. - Migrates debugger output and JSON tests.
| File | Description |
|---|---|
types.go |
Exports new result aliases and errors. |
session.go |
Updates session execution contract. |
runtime.go |
Updates runtime execution ownership. |
result/types.go |
Defines output and content contracts. |
result/types_test.go |
Tests content JSON behavior. |
result/errors.go |
Adds lifecycle sentinels. |
result/doc.go |
Documents result package semantics. |
README.md |
Documents usage and migration. |
output_examples_test.go |
Tests streaming examples. |
output_example_test.go |
Adds executable usage examples. |
output_contract_test.go |
Verifies aliases and caller behavior. |
output_contract_fixture_test.go |
Adds scripted output fixtures. |
options.go |
Clarifies codec selection. |
doc.go |
Documents root output lifecycle. |
debugger/types.go |
Migrates event output to content. |
debugger/session.go |
Clarifies event ownership. |
debugger/doc.go |
Documents detached event content. |
debugger/content_test.go |
Tests debugger content serialization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+107
to
+110
| // Close is safe concurrently and idempotent, returning the recorded cleanup | ||
| // outcome, usually nil, even after finalization. Repeated calls need not | ||
| // return identical error-wrapper pointers. Being closed is not itself a | ||
| // Close error. It does not close caller-owned sessions or borrowed parents. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a major refactor to the output and content handling across the API, especially for query execution and debugger events. The core change is the migration from materialized output structs to a new model where
Outputis a one-shot handle, and actual data is accessed through explicit consumption (Consume,Collect) resulting in detached, immutableContentobjects. This affects the Go API, JSON representations, and debugger event contracts. The changes clarify ownership, error handling, and concurrency, and provide new documentation and tests to support the new model.API and Output Handling Refactor
Runtime.RunandSession.Runnow return a caller-owned, one-shotOutputhandle; actual data is accessed viaConsumeorCollect, and output presence/errors are handled separately from execution errors. The new model is thoroughly documented inREADME.mdanddoc.go. [1] [2] [3]Outputstruct is replaced by a detachedContentstruct for materialized data, with clear rules for presence, absence, and error reporting. The JSON encoding for output is changed to a nestedmetadata/dataobject, and live output handles must not be serialized. [1] [2]Debugger Event Model Update
*Content, not live output handles, ensuring that reading an event does not consume output or require retaining a session. This is reflected in theEventstruct and associated documentation. [1] [2] [3]Options and Content Type Handling
WithOutputContentTypeandSetOutputContentTypenow refer to the encoded representation's media type, which is reported byOutput.Metadata().ContentType. Codec availability is validated during encoding and surfaced through consumption errors. [1] [2] [3] [4]Serialization and Migration Details
README.md. [1] [2]These changes modernize and clarify output handling, making resource ownership and error propagation more explicit and robust.